feat(sight): add grader dashboard controls#1396
Conversation
|
Frontend reads cleanly overall. Two small things plus a coordination note:
Coordination: this PR calls |
|
Updated in cdee9bf to address the two dashboard review notes:
Added regression coverage for:
Validation:
I also confirmed that the remaining full |
cdee9bf to
3018079
Compare
jfeng18
left a comment
There was a problem hiding this comment.
Reviewed the diff end-to-end (independent sweep + adversarial verification, plus a local npm ci + vitest run of the grader tests — 55 pass). The cross-component contract with the #1395 backend is clean: verdict / root_cause / evidence_type / grader_type serialization, optional-vs-null semantics, the 409 conversation_not_ready shape, /grader/latest returning a bare result or JSON null, and the request body all line up field-for-field. Both new effects converge (no re-fetch loop; the AtifViewer mount effect stays []-gated so widening handleLoad's deps is loop-safe). Nice split.
Two low, non-blocking items:
1. [low] 7 of 12 interruption types render as raw English in the grader panel. findingLabel (EvaluationPanel.tsx:278) and evidenceLabel (:320, via its findingLabel fallback) only translate 5 interruption codes (llm_error, sse_truncated, network_timeout, service_unavailable, agent_crash). But interruption-derived findings carry finding.code = record.interruption_type (grader/rule.rs:277) and their evidence carries label = interruption_type (grader/evidence.rs:155), and the full vocabulary is 12 — all real emitted variants in InterruptionType::as_str (interruption/types.rs:41-51). So findings/evidence for auth_error, context_overflow, dead_loop, rate_limit, retry_storm, safety_filter, token_limit fall through labels[value] ?? value and show raw snake_case, even though apiClient.ts already exports INTERRUPTION_TYPE_CN with Chinese for all of them (and InterruptionBadge already renders those same types translated). Consulting INTERRUPTION_TYPE_CN in the fallback would close the gap.
2. [nit] Duplicate React key in the findings list. key={${finding.code}-${finding.message}} (EvaluationPanel.tsx:160). build_findings emits one finding per interrupted event, and one per unresolved interruption of the same type, so (code, message) is not unique — 2+ such findings produce identical keys and React logs "Encountered two children with the same key". I verified all rows still render on initial mount (no dropped rows), so this is a console-warning / reconciliation-hygiene nit, not a visible bug. A per-item index or a ref id in the key clears it.
Everything else LGTM.
chengshuyi
left a comment
There was a problem hiding this comment.
Code Review
整体代码结构清晰,组件拆分合理,测试覆盖充分(95 tests pass)。类型定义完整且与后端 schema 对齐良好。以下是几点建议:
🔴 Finding 1(建议必须修复)— evaluateConversation 未使用 apiFetch 封装
apiClient.ts 中 fetchLatestEvaluation 使用了项目统一的 apiFetch,但 evaluateConversation 直接调用原生 fetch:
const res = await fetch(`${API_BASE}/api/grader/evaluate`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ ... }),
});apiFetch 封装了两个认证关键逻辑:
credentials: "same-origin"— 确保携带 session cookie- 401 状态码自动跳转登录页
绕过 apiFetch 意味着在 server.auth.enabled = true 的环境下,POST 请求不会携带 cookie,后端返回 401 后用户也不会被引导到登录页,"开始评估"按钮会直接失败。
建议:扩展 apiFetch 支持 POST(如新增 apiFetchPost<T>(url, body)),或让 evaluateConversation 复用相同的 credentials 和 401 处理逻辑。
🟡 Finding 2 — EvaluationPanel 的 initialResult prop 更新后不生效
const [result, setResult] = useState<EvaluationResult | null>(initialResult);useState(initialValue) 只在首次 mount 时使用初始值,后续 prop 变化不会同步。当前因为面板展开/收起会重新挂载,影响有限,但如果将来需要在面板打开状态下更新数据会出问题。建议添加 useEffect 监听 initialResult 变化。
🟡 Finding 5 — searchParams 加入 useEffect 依赖可能导致不必要的重新请求
AtifViewerPage.tsx 中将 searchParams 加入了 fetch ATIF 文档的 useEffect 依赖数组,但 effect 内部又会调用 setSearchParams,形成潜在的循环触发。建议从依赖中移除 searchParams,改用 useRef 或在回调体内读取最新值。
🟢 其他小建议
highlightedSections中doc.steps缺少空值防御,建议加doc.steps ?? []- Evidence ref 的 React key 可能不唯一,建议加上数组 index 作为 tiebreaker
EvaluationPanel.tsx中约 140 行是独立的 label 映射函数,可考虑统一到单一 i18n 对象中(不阻塞合入)
Add conversation-list controls, latest-run badges, and a detail panel for manually running and inspecting grader results. Keep the frontend split from the Rust API implementation so the dashboard surface can be reviewed independently. Assisted-by: Codex:GPT-5 Signed-off-by: wanghao <1562495626@qq.com>
Only successful latest-run lookups mark conversations as fetched. Failed lookups now surface a retryable badge state instead of hiding errors. Unknown backend verdict and root-cause values fall back to raw labels and server actions. This keeps the dashboard compatible with small grader API enum extensions. Assisted-by: Codex:GPT-5 Signed-off-by: wanghao <1562495626@qq.com>
Reuse the authenticated API client for evaluation POST requests so grader actions carry session credentials and share 401 handling. Synchronize incoming results, translate interruption findings, and use unique list keys. Keep ATIF URL loading single-shot and tolerate missing step arrays. The dashboard API contract and endpoint shapes remain unchanged. Assisted-by: Codex:GPT-5 Signed-off-by: wanghao <1562495626@qq.com>
3018079 to
6dcfb9a
Compare
|
Updated in Review feedback addressed:
Validation:
|
jfeng18
left a comment
There was a problem hiding this comment.
Re-reviewed 6dcfb9a1 — all the raised points are resolved and I verified them against a local build (npm ci, 5 grader test files → 103 passing, tsc --noEmit clean):
- Duplicate finding/evidence keys — now unique (
-${index}); confirmed with a discriminating test (two findings sharingcode+messageno longer emit React's duplicate-key warning). - Interruption i18n —
findingLabelnow falls back toINTERRUPTION_TYPE_CN, so the 7 previously-untranslated types (rate_limit,auth_error, …) render in Chinese;evidenceLabelinherits it. - @chengshuyi's apiFetch point —
evaluateConversationnow goes through the sharedapiFetch, so grader mutations carrycredentials: same-origin+ the 401→login redirect, and 409conversation_not_readymaps cleanly via the new structuredApiRequestError. The refactor is backward-compatible for the existing GET/POST/DELETE callers (initdefaults to{},credentialsis forced last). initialResultsync, thesteps ?? []guards, and the ATIF ref-based URL read are all in and test-covered; the deeplink-highlight path still passes.
Contract with the #1395 backend remains field-aligned. LGTM — approving. @chengshuyi feel free to confirm the apiFetch change reads right on your end.
Description
Add dashboard controls for the conversation grader. The UI can run manual evaluations, show latest-run badges in the conversation list, and display score dimensions, findings, root cause, and evidence links in a detail panel.
This PR is frontend-only and split from the grader backend so the dashboard interaction surface can be reviewed separately.
Related Issue
Refs #1304
Type of Change
Scope
cosh(copilot-shell)cosh-ng(cosh-ng)sec-core(agent-sec-core)skill(os-skills)sight(agentsight)tokenless(tokenless)ckpt(ws-ckpt)memory(agent-memory)anolisa(anolisa-cli)skillfs(SkillFS)Checklist
cosh: Lint passes, type check passes, and tests passcosh-ng:cargo clippy --all-targets -- -D warningsandcargo fmt --checkpasssec-core(Rust):cargo clippy -- -D warningsandcargo fmt --checkpasssec-core(Python): Ruff format and pytest passskill: Skill directory structure is valid and shell scripts pass syntax checksight: dashboard typecheck and tests passtokenless:cargo clippy -- -D warningsandcargo fmt --checkpassmemory(Linux only):cargo clippy --all-targets -- -D warnings,cargo fmt --check, andcargo testpassanolisa:cargo clippy --all-targets --locked -- -D warnings,cargo fmt --all --check, andcargo test --lockedpassskillfs:cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, andcargo test --workspacepasspackage-lock.json/Cargo.lock)Testing
Ran on the Linux AgentSight test host from an archive of this split branch:
npm cinpm run typechecknpm test -- src/test/apiClient.test.ts src/test/EvaluationBadge.test.tsx src/test/EvaluationPanel.test.tsx src/test/ConversationList.test.tsx src/test/AtifViewerPage.test.tsxResult: 5 test files passed, 95 tests passed.
Additional Notes
Split from #1349 following review feedback. The backend API is intentionally left to a separate PR.